-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make COOP+COEP do not imply crossOriginIsolated. #6098
Make COOP+COEP do not imply crossOriginIsolated. #6098
Conversation
I think it's good to make BCG's cross-origin isolated and agent cluster's cross-origin isolated platform independent, and cross-origin isolated capability platform dependent. That means even on Android WebView document.domain mutation is no-op when COOP: same-origin and COEP: require-corp is specified. What do you think? |
I agree it would be a shame not restricting document.domain. I have been a bit confused by all the different cross-origin-isolated concept. So we have:
I guess I should just add a platform-specific exception to: |
My recommendation would be...
|
I agree with @yutakahirano's suggestions. To give more detail on a particularly tricky part:
My suggestion concretely here is that before step 4, you establish a boolean. Something like:
then you can modify the "cross-origin isolated capability" to be something like:
Without establishing the boolean ahead of time, if you just wrote something like "return false at the user agent's discretion" in the COIC algorithm, it would be possible per-spec for the value to change over time. |
Thanks Yutaka and Domenic, this is really helpful! |
@ArthurSonzogni , what's the status of this bug? |
I just need to apply Domenic suggestions and yours. Nothing looks hard, I just need to produce some work ;-) I was focused on my main project. Enabling COOP+COEP on every platforms is a side project that I wanted to see progress too, but this isn't my main priority and nobody really depends on this. Maybe you do somehow? I've booked some time Wednesday for fixing this. I will update this thread. |
Thank you! |
The section linked above is called once for every "window" object being created. So this doesn't really represent a "global" per-agent variable. Maybe there is another section that is called only once, where I can put this variable instead? (Or maybe I can just add a note explaining the agent have to make a decision and not change its mind over time. |
I don't know if it's important for the canBeCOI boolean to be the same across the entire agent. Having it be per-window seems reasonable, since it's just applying an implementation-defined restriction on top of the existing agent cluster-wide cross-origin-isolated situation. |
590a022
to
c79e8f3
Compare
I made an update. It's a bit unfortunate that "canBeCOI" can't be made more global. I am defining from "Script settings for Window objects", but I also need to use it from the part about workers. I would be happy to get some feedback from there. I still need to do the suggested change in the w3c/ServiceWorker repository. |
If we wanted it to be more global, we could modify the agent cluster's cross-origin-isolated at the time that gets set (which is roughly at agent cluster creation time). However, I haven't thought through the full implications of that, and am unsure it's a great idea. I remember feeling that the current design you've uploaded is good, when we discussed this 26 days ago. I'm currently attending BlinkOn so my time for nontrivial reviews like this one is a bit scarce. But as soon as I'm able (probably Friday?) I'll do a review of the latest changes you've made, and also think more deeply about whether it would be a good idea to make it change the agent cluster's cross-origin isolated. Thoughts from @yutakahirano or @annevk would be welcome in the meantime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach is fine. If you wanted something more global you would have to change the browsing context group and agent cluster's cross-origin isolated concept to a tri-state I think, but I'm not sure what the observable differences would be. I would hope none.
Hmm, thinking about it more that tri-state would be a lot cleaner as it more closely matches what an implementation would want to do. It would not process-isolate the agent cluster after all. I think it's worth another look to see how hard that would be. |
That was my feeling since the beginning. I would be happy to do this. So, I guess I would need to modify: Replace:
by:
Replace:
By:
What do you think of this in general? |
Yeah, I think that's right. And note that aside from the browsing context group's cross-origin isolated, you'll also need to do this for the similar boolean on agent cluster (which just copies the value from the browsing context group it's associated with). Edit: I would try to avoid the word "process" to not bake particular computer architectures too much into the design, even though they are somewhat in the end. |
@annevk : The patch with your suggestions from #6098 (comment) is ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good overall. My concerns are mainly about something that was already there (SAB protection) and the exact wording to use in the various places that need changes, but the architecture seems sound to me.
source
Outdated
@@ -8078,7 +8078,8 @@ interface <dfn>DOMStringList</dfn> { | |||
<span>agent cluster</span>.</p></li> | |||
|
|||
<li> | |||
<p>If <var>agentCluster</var>'s <span>cross-origin isolated</span> is false, then throw a | |||
<p>If <var>agentCluster</var>'s <span>cross-origin isolated</span> is not <code | |||
data-x="bcg-isolation-physic">isolation-physic</code>, then throw a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this isn't checking the capability. Wouldn't that mean that a cross-origin document can use SharedArrayBuffer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now you say it, I am surprised too. My understanding is that we should check the capability.
That's what we implemented on Chrome here:
https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/execution_context/execution_context.cc;l=184;bpv=1;bpt=1?q=sharedarraybuffertran
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, this requires talking about the destination realms.
I had some questions about it previously: https://crbug.com/1144838
Here is a copy of my questions:
I discussed:
https://chromium-review.googlesource.com/c/chromium/src/+/2343658
with sigurds@. It made me ask myself several questions, that I answered by adding tests:
https://chromium-review.googlesource.com/c/chromium/src/+/2514211
I can now say the following:
From
- a COOP/COEP page,
- an OS without SharedArrayBuffer enabled by default (e.g. Android),
Then it is possible to share a SharedArrayBuffer from a non crossOriginIsolated document iframe.
Specifically, we have:
1. (sender.crossOriginIsolated == false) + (receiver.crossOriginIsolated == false) => Blocked.
2. (sender.crossOriginIsolated == true ) + (receiver.crossOriginIsolated == true ) => Transfered
3. (sender.crossOriginIsolated == true ) + (receiver.crossOriginIsolated == false) => Blocked
4. (sender.crossOriginIsolated == false) + (receiver.crossOriginIsolated == true ) => Transfered
This is not necessary bad, what matters is the receiver.
I wanted to make sure you are okay with case 4. WDYT?
I made some tests here:
https://chromium-review.googlesource.com/c/chromium/src/+/2514211
Unfortunately, not WPT test, because this rely on flags not enabled yet.
In the spec, I don't know the destination realms.
I guess I should update:
StructuredSerializeInternal ( value, forStorage [ , memory ] )
into
StructuredSerializeInternal ( value, forStorage [ , memory ] [, destinationRealm] )
and plumb |destinationRealm| everywhere this function is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up moving this part toward the deserialization, because targetRealm was already plumbed. I didn't wanted to plumb destinationRealm on the serialization side. Does this sound good to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should split this part out into its own PR given it's quite separate from what this PR is doing. Having said that, I think we can also check this when serializing as there is a current Realm. The difference here is whether postMessage()
throws or the target gets a messageerror
event. I suspect we want postMessage()
to throw, though I also don't care too strongly. I was hoping @domenic and @yutakahirano would chime in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, woah, let's not make changes to structured serialization in this PR, especially one that changes the signature. That's a big change that needs more discussion in a dedicated issue, not a comment thread on a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it out.
source
Outdated
<span>agent cluster</span> are automatically origin-isolated. The `<code | ||
<p class="note">Similarly, <code>Document</code>s with <span>agent cluster</span>'s | ||
<span>cross-origin isolated</span> <code data-x="bcg-isolation-physic">isolation-physic</code> | ||
are automatically origin-isolated. The `<code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all the not-"none" values offer the same features as Origin Isolation in terms of what is observable to web developers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are close. The observable differences I have in mind are:
- difference when opening a new window from a sandboxed iframe. The opener can't influence the openee with sandbox-flags. I think this result in error page.
- difference, because we might swap browsing context group during a navigation, depending on the various headers.
Outside of navigations
68d940c
to
9c0351e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In penance for the long delays in review, I'll help out by pushing some commits to fix the issues I mention in this review, plus some editorial touchups.
The [specification] currently requires [COOP] + [COEP] to give access to crossOriginIsolated capabilities like SharedArrayBuffer. Some platforms can't easily support multiple processes (like Android Webview). Therefore, they can't really support crossOriginIsolated. However the are no strong reasons for them not to enforce COEP (and maybe COOP) when their associated headers are present. It would be great enforcing COEP (and maybe COOP) on all platforms, desptie the lack of crossOriginIsolated capabilities. This patch makes the specification to allow (instead of requiring) platform to set the crossOriginIsolated flag when both COOP and COEP are used. Setting crossOriginIsolated becomes platform dependent. In exchange, we can enforce COEP (and COOP) in a non platform dependent way, without conflicting with the specification about crossOriginIsolated. [Bug]: whatwg#6060 [specification]: https://html.spec.whatwg.org/#cross-origin-opener-policies [COOP]: https://html.spec.whatwg.org/#cross-origin-opener-policy [COEP]: https://html.spec.whatwg.org/#coep
485b55e
to
f28344f
Compare
@annevk @yutakahirano @ArthurSonzogni PTAL. My changes are:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that we would split out the structured serialization bug fix entirely, but I guess it's okay to include it here if we spell it out in the commit message. I found a couple nits, seems okay to me to land this modulo those, but I'd appreciate one more review since it's a bit harder to review on a small screen.
@@ -80423,9 +80462,6 @@ interface <dfn>BarProp</dfn> { | |||
a registrable domain suffix of and is not equal to</span> <var>effectiveDomain</var>, then throw | |||
a <span>"<code>SecurityError</code>"</span> <code>DOMException</code>.</p></li> | |||
|
|||
<li><p>If the <span>surrounding agent</span>'s <span>agent cluster</span>'s <span>cross-origin | |||
isolated</span> is true, then return.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removed? Is it redundant with is origin-keyed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. Another thing to spell out in the commit message.
<p class="note">It is difficult on some platforms to provide the security properties required by | ||
the <span data-x="concept-settings-object-cross-origin-isolated-capability">cross-origin | ||
isolated capability</span>. Only "<code | ||
data-x="cross-origin-isolation-concrete">concrete</code>" might grant access to it. "<code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/might/will/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using "might" was intentional. This also depends on the feature policy bit.
For instance: BCG cross-origin-isolation-concrete
+ document using Feature-Policy: cross-origin-isolated 'none'
won't grant the window.crossOriginIsolated
bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my last patchset. I used can
. It sounds less confusing than might
, and more precise than will
.
isolated capability</span>. Only "<code | ||
data-x="cross-origin-isolation-concrete">concrete</code>" might grant access to it. "<code | ||
data-x="cross-origin-isolation-logical">logical</code>" won't, and is used by implementations on | ||
other platforms.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This other is weird, since we already started with some platforms that might use "logical".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I removed ,and is used by implementations on other platforms
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @domenic for the big update!
<p class="note">It is difficult on some platforms to provide the security properties required by | ||
the <span data-x="concept-settings-object-cross-origin-isolated-capability">cross-origin | ||
isolated capability</span>. Only "<code | ||
data-x="cross-origin-isolation-concrete">concrete</code>" might grant access to it. "<code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my last patchset. I used can
. It sounds less confusing than might
, and more precise than will
.
isolated capability</span>. Only "<code | ||
data-x="cross-origin-isolation-concrete">concrete</code>" might grant access to it. "<code | ||
data-x="cross-origin-isolation-logical">logical</code>" won't, and is used by implementations on | ||
other platforms.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I removed ,and is used by implementations on other platforms
.
@ArthurSonzogni could you prepare a tentative commit message that also accounts for the additional changes as discussed above and post it as a comment? I think this is good to go. (I pushed one more nit.) |
Thanks @annevk ! Tentative commit message:
|
Thanks, we should also mention the change to StructuredSerializeInternal, but I can merge that in as well. If there are no further comments I'll merge this tomorrow. |
Did we change StructuredSerializeInternal? I see only some "data-x=" being now properly set. The content is the same. |
It went from checking cross-origin isolated to cross-origin isolated capability, right? That's a big change, even if one could consider it an oversight from when we introduced cross-origin isolated capability. |
Indeed! Updated tentative commit message. (Feel free to improve it, if needed)
|
Thanks so much @ArthurSonzogni! |
Thanks! |
The specification currently requires COOP + COEP to give access to
crossOriginIsolated capabilities like SharedArrayBuffer.
Some platforms can't easily support multiple processes (like Android
Webview). Therefore, they can't really support crossOriginIsolated.
However the are no strong reasons for them not to enforce COEP (and
maybe COOP) when their associated headers are present.
It would be great enforcing COEP (and maybe COOP) on all platforms,
desptie the lack of crossOriginIsolated capabilities.
This patch makes the specification to allow (instead of requiring)
platform to set the crossOriginIsolated flag when both COOP and COEP are
used.
Setting crossOriginIsolated becomes platform dependent. In exchange, we
can enforce COEP (and COOP) in a non platform dependent way, without
conflicting with the specification about crossOriginIsolated.
/browsers.html ( diff )
/origin.html ( diff )
/structured-data.html ( diff )
/webappapis.html ( diff )
/window-object.html ( diff )
/workers.html ( diff )